Emit a reasonable error if someone uses type in place where expression is exprected #4411
Conversation
vlstill
force-pushed
the
type-checking-type-expr
branch
from
d231c97
to
5566a05
Compare
vlstill
force-pushed
the
type-checking-type-expr
branch
from
5566a05
to
78f45bd
Compare
|
What is the error you get when you try this within in the parser? Or as default argument for a parameter. |
In parser state, for a parser parameterized by In a default argument in a package: Frankly, I'm surprised how much does the P4 parser take the context into account (i.e. that H is a type variable in these locations)! P4 code: parser pars<H>(in bit<16> buf, out H hdrs) {
bool var;
state start {
var = buf > H;
}
}
control ctrl<H>(bool val);
package pkg<H>(
ctrl<H> val = ctrl(H > 3)
);I could commit those as tests too if you want (they would be separate files thought as the parsing errors cause the compiler to stop right there). |
|
Okay, was just wondering if this error is only reproducible in actions. It can't hurt to add these tests for future front ends. |
| @@ -3052,6 +3052,9 @@ const IR::Node *TypeInference::postorder(IR::PathExpression *expression) { | |||
| if (type != nullptr) | |||
| // may be nullptr because typechecking may have failed | |||
| type = cloneWithFreshTypeVariables(type->to<IR::Type_MethodBase>()); | |||
| } else if (decl->is<IR::Type_Declaration>()) { | |||
There was a problem hiding this comment.
I am wondering whether this might not be too restrictive. On the other hand a PathExpression reference to a Type_Declaration should probably parsed as Type_Name
There was a problem hiding this comment.
Exactly my thought. And a PathExpression is an expression, so it can be used to build more complex expressions. Having a type there directly does not make much sense.
There was a problem hiding this comment.
Any name that is in-scope as a type will be lexed as a TYPE_IDENTIFIER and recognized as a typeName in the parser, so should create a Type_Name rather than a Path. The exception is when the type is used before its declaration, which (I think) can only happen with type parameters.
| @@ -3052,6 +3052,9 @@ const IR::Node *TypeInference::postorder(IR::PathExpression *expression) { | |||
| if (type != nullptr) | |||
| // may be nullptr because typechecking may have failed | |||
| type = cloneWithFreshTypeVariables(type->to<IR::Type_MethodBase>()); | |||
| } else if (decl->is<IR::Type_Declaration>()) { | |||
There was a problem hiding this comment.
Any name that is in-scope as a type will be lexed as a TYPE_IDENTIFIER and recognized as a typeName in the parser, so should create a Type_Name rather than a Path. The exception is when the type is used before its declaration, which (I think) can only happen with type parameters.
vlstill
force-pushed
the
type-checking-type-expr
branch
from
5a37a0a
to
5fadbaa
Compare
Found by fuzzy testing. Fuzzer created an annotation that contained something like
val=4<H>2whereHis a type variable.Interestingly, I was not able to reproduce this with p4test, I am getting parsing errors there (both when trying to use type name in annotation and in expression in a control).It seems like the only place where this can trigger is in an annotation on the thing that is parametrized by the type variable. The annotation is seemingly (and for parser) before the variable declaration, butfindDeclarationfinds it, probably since the declaration is semantically inside the object that introduces the variable. This is a very niche case, but still it should not hit a BUG as it did before (type-in-expr.p4(4): Could not find type of <Type_Var>(201) H/7 H/7).